o/ifacestate: properly handle undo scenarios where auto-connections are dropped - #17018
Merged
andrewphelpsj merged 6 commits intoMay 11, 2026
Merged
Conversation
andrewphelpsj
force-pushed
the
fix-connection-undo
branch
from
May 4, 2026 21:56
8447578 to
35bc2c7
Compare
|
Mon May 11 15:19:58 UTC 2026 Failures:Preparing:
Executing:
Restoring:
Skipped tests from snapd-testing-skipIf you wish to have any of the below tests run in your PR, in your PR description, add 'unskip:' followed by a copy-and-pasted list (without variants) of the below tests you wish to run (unskip plus test list must be valid yaml)
|
andrewphelpsj
force-pushed
the
fix-connection-undo
branch
from
May 4, 2026 21:58
35bc2c7 to
5a5154a
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #17018 +/- ##
==========================================
+ Coverage 79.04% 79.07% +0.02%
==========================================
Files 1375 1376 +1
Lines 191012 191330 +318
Branches 2465 2465
==========================================
+ Hits 150981 151285 +304
- Misses 30924 30933 +9
- Partials 9107 9112 +5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
pedronis
reviewed
May 5, 2026
| originalConns[connID] = connState | ||
| } | ||
|
|
||
| task.Set("original-connection-states", originalConns) |
Contributor
There was a problem hiding this comment.
this is a strange name as this is not the full set of original connections, not we want it to be that
Member
Author
There was a problem hiding this comment.
I updated this, and renamed a few other things. Using changed-connection-snapshot now.
andrewphelpsj
commented
May 5, 2026
| // The connection refers to a plug or slot that doesn't exist anymore, e.g. because of a refresh | ||
| // to a new snap revision that doesn't have the given plug/slot. | ||
| if plugInfo == nil || slotInfo == nil { | ||
| // automatic connection can simply be removed (it will be re-created automatically if needed) |
…tions are dropped
bboozzoo
approved these changes
May 5, 2026
bboozzoo
left a comment
bboozzoo
left a comment
Contributor
There was a problem hiding this comment.
LGTM, had a chat with @andrewphelpsj to discuss the details
…tions are dropped
Meulengracht
reviewed
May 6, 2026
Comment on lines
+380
to
+384
| if task.Status() != state.UndoingStatus { | ||
| if err := snapshotChangedConnectionsForUndo(task, snapName, changedConns); err != nil { | ||
| return nil, nil, err | ||
| } | ||
| } |
Member
There was a problem hiding this comment.
I know this is super convenient place to put this code, it creates some implicit behaviour of this function that is really not obvious, and should as a minimum be documented. Ideally I'd rather see this as a return value an handled on a task level instead, but I recognize the annoying thing about changing the call chain that also goes through setupProfilesForAppSet
Member
There was a problem hiding this comment.
Just a completely naive thought: is there any reason we don't just cache the all the refreshed snaps connections from "conns" state at the start of Do and restore this during undo?
Contributor
There was a problem hiding this comment.
This isn't wrong, it would be much more explicit. The catch is we don't know which ones are relevant until quite late, so we'd need to collect all of the connections of a snap, along with information on how they were established. Not great but doable. For some snaps, like snapd there's quite a lot of data to be stored this way.
Member
Author
There was a problem hiding this comment.
I pushed a commit renaming some things and adding some comments. I agree that making the task handler itself store this data on the task would be nice, but that would require a pretty large refactor I think?
Member
There was a problem hiding this comment.
I don't think it's that large a refactor, but it's also something we can do at another stage.
Comment on lines
+709
to
+711
| if err := restoreConnectionsForSetupProfiles(task); err != nil { | ||
| return err | ||
| } |
Member
There was a problem hiding this comment.
I think this should mention that it relies on the task variable being set by refreshAppSetConnections during the do path.
Mohit-Chachada
approved these changes
May 6, 2026
pedronis
reviewed
May 6, 2026
| func (m *InterfaceManager) reloadConnections(snapName string) (reloadedConnectionIDs []string, err error) { | ||
| // The return value is the list of reloaded connection IDs, plus the original | ||
| // connection states whose persisted state was changed. | ||
| func (m *InterfaceManager) reloadConnections(snapName string) (reloadedConnectionIDs []string, changedConns map[string]*schema.ConnState, err error) { |
Contributor
There was a problem hiding this comment.
I suppose changedConns includes also dropped connections?
Contributor
There was a problem hiding this comment.
Member
Author
There was a problem hiding this comment.
Yes, it contains both deleted connections and connections whose static attributes were updated.
andrewphelpsj
force-pushed
the
fix-connection-undo
branch
from
May 6, 2026 14:54
e8f9829 to
d56f83c
Compare
Meulengracht
approved these changes
May 8, 2026
Comment on lines
+380
to
+384
| if task.Status() != state.UndoingStatus { | ||
| if err := snapshotChangedConnectionsForUndo(task, snapName, changedConns); err != nil { | ||
| return nil, nil, err | ||
| } | ||
| } |
Member
There was a problem hiding this comment.
I don't think it's that large a refactor, but it's also something we can do at another stage.
andrewphelpsj
added
the
Auto rerun spread
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
When a refresh moves to a revision that drops an auto-connected plug,
reloadConnectionsremoves that auto-connection from the connection state. If the refresh fails, the deleted auto-connection is not restored. This leaves the interface disconnected after the undo has completed.This only impacts auto-connections because manual connections whose plug or slot is missing in the new revision are kept in the connection state, but auto-connections are pruned.
A similar idea applies to static plug and slot attributes from the new revision. If the refresh fails, rollback restores the old snap revision, but the new revision's static attributes are kept in the connection state.
To solve this problem, this PR updates
setup-profilesso that it keeps track of connection state changes and restores them in the undo handler.